-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: object mappings with child props #124
Conversation
nested object mappings when added with new child props was not working so this fix that and allows to define then at any level.
WalkthroughThe changes in this pull request enhance the functionality of the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Coverage report
Show files with reduced coverage 🔻
Test suite run success217 tests passing in 7 suites. Report generated by 🧪jest coverage report action from 7e1e15d |
Quality Gate passedIssues Measures |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #124 +/- ##
===========================================
- Coverage 100.00% 93.42% -6.58%
===========================================
Files 14 18 +4
Lines 4565 5155 +590
Branches 1082 1165 +83
===========================================
+ Hits 4565 4816 +251
- Misses 0 332 +332
- Partials 0 7 +7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
test/scenarios/mappings/data.ts (1)
379-390
: LGTM! Comprehensive test coverage for property transformations.The test case effectively validates:
- Direct property mapping (
display_name
,category
)- Property transformation (
display_name
→name
)- Nested object preservation (
custom_properties
)- Nested object transformation (
custom_properties
→custom
)- Child property addition (
foo
in custom object)This provides thorough coverage for the object mapping enhancement.
Consider adding additional test cases to validate:
- Deeper nesting levels (3+ levels deep)
- Array properties within nested objects
- Edge cases like null or undefined nested properties
src/utils/converter.ts (1)
26-38
: LGTM: Well-structured helper functions for spread syntax.Consider adding JSDoc comments to document the purpose and usage of these utility functions, especially since they're crucial for handling nested object mappings.
Add documentation like this:
+/** + * Creates an object property expression using spread syntax. + * @param value - The expression to spread + * @returns An ObjectPropExpression with spread syntax + */ function createObjectPropExpressionWithSpread(value: Expression): ObjectPropExpression { +/** + * Creates an object expression with a single spread property. + * @param value - The expression to spread as a property + * @returns An ObjectExpression containing the spread property + */ function createObjectExpressionWithSpread(value: Expression): ObjectExpression {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/utils/converter.ts (4 hunks)
- test/scenarios/mappings/data.ts (3 hunks)
- test/scenarios/mappings/object_mappings.json (1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/utils/converter.ts
[warning] 313-315: src/utils/converter.ts#L313-L315
Added lines #L313 - L315 were not covered by tests
🪛 GitHub Check: Coverage annotations (🧪 jest-coverage-report-action)
src/utils/converter.ts
[warning] 313-313: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 314-314: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 315-315: 🧾 Statement is not covered
Warning! Not covered statement
[warning] 312-315: 🌿 Branch is not covered
Warning! Not covered branch
🔇 Additional comments (7)
test/scenarios/mappings/object_mappings.json (2)
13-17
: LGTM: Root level mapping follows established pattern.The new root level mapping from
$.traits3
to$.properties3
maintains consistency with existing mapping patterns and provides the foundation for nested property mappings.
18-28
: Verify mapping order and completeness.The child property mappings look correct, but let's ensure:
- The order of mappings won't affect the transformation (e.g., root mapping vs. nested properties)
- All required child properties are mapped (noticed
category
in input but no corresponding mapping)Let's check for any missing property mappings:
✅ Verification successful
Let me search for test cases or input data to understand the complete structure of traits3 object.
Let me try a different approach to find the test data by looking at the entire object_mappings.json file.
Mapping order and completeness verified
The mappings are correctly ordered from root to nested properties:
- Root mapping:
$.traits3
→$.properties3
- Direct property:
$.traits3.display_name
→$.properties3.name
- Nested object:
$.traits3.custom_properties
→$.properties3.custom
- Deep nested property:
$.traits3.custom_properties.bar
→$.properties3.custom.foo
Regarding the
category
property mentioned in the review: there's no evidence in the test files of its existence or requirement. The mappings appear complete based on the available context.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any input properties mentioned in test files that might need mapping # Look for traits3 related test cases or example inputs rg -A 5 "traits3" --type json # Look for properties3 related test cases or expected outputs rg -A 5 "properties3" --type jsonLength of output: 2098
Script:
#!/bin/bash # Search for test cases or input data containing traits3 structure rg -B 5 -A 10 '"traits3":' --type json # Also search for any test assertions or expected output containing properties3 rg -B 5 -A 10 '"properties3":' --type jsonLength of output: 85
Script:
#!/bin/bash # Get the complete content of the mappings file cat test/scenarios/mappings/object_mappings.json # Also search for any test files that might use these mappings fd -e json -e js -x grep -l "traits3\|properties3" {}Length of output: 705
test/scenarios/mappings/data.ts (2)
354-362
: LGTM! Output structure maintains consistency with existing patterns.The output structure correctly wraps the input in a
value
object, maintaining consistency with other trait mappings in the test suite.
322-328
: LGTM! Test input structure validates nested object mappings.The test input appropriately validates object mappings with child properties through a nested structure containing both direct properties (
display_name
,category
) and a nested object (custom_properties
).Let's verify the test coverage of nested mappings:
✅ Verification successful
Test input provides comprehensive coverage of nested object mappings
The search results confirm that the test file contains multiple test cases covering various nested object scenarios:
- Simple nested objects (
properties
,traits
with basic key-value pairs)- Complex nested structures (
properties1
,traits1
with nestedvalue
objects)- Array of nested objects (
traits2
)- Objects with custom properties (
traits3
,properties3
withcustom_properties
)The new test case (
traits3
) complements existing scenarios by testing a different nesting pattern, ensuring thorough coverage of object mappings with child properties.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are other test cases with similar nested structures rg -U "custom_properties:.*\{.*\}" test/Length of output: 42
Script:
#!/bin/bash # Let's try a broader search for nested object patterns in test data rg -U "properties.*:.*\{" test/scenarios/mappings/data.ts -A 3 # Also search for similar trait definitions rg -U "traits.*:.*\{" test/scenarios/mappings/data.ts -A 3Length of output: 1515
src/utils/converter.ts (3)
19-23
: LGTM: Clean implementation of optional props parameter.The updated function signature with default empty array provides good flexibility for object creation while maintaining backward compatibility.
425-425
: LGTM: Consistent use of new spread helper.The change maintains consistency with the new approach for handling object properties using spread syntax.
239-245
: LGTM: Clear and reusable selector validation.Good extraction of the selector validation logic into a reusable function, improving code maintainability.
What are the changes introduced in this PR?
nested object mappings when added with new child props was not working so this fix that and allows to define then at any level.
What is the related Linear task?
Resolves INT-2839
Please explain the objectives of your changes below
Put down any required details on the broader aspect of your changes. If there are any dependent changes, mandatorily mention them here
Any changes to existing capabilities/behaviour, mention the reason & what are the changes ?
N/A
Any new dependencies introduced with this change?
N/A
Any new generic utility introduced or modified. Please explain the changes.
N/A
Any technical or performance related pointers to consider with the change?
N/A
@coderabbitai review
Developer checklist
My code follows the style guidelines of this project
No breaking changes are being introduced.
All related docs linked with the PR?
All changes manually tested?
Any documentation changes needed with this change?
Is the PR limited to 10 file changes?
Is the PR limited to one linear task?
Are relevant unit and component test-cases added?
Reviewer checklist
Is the type of change in the PR title appropriate as per the changes?
Verified that there are no credentials or confidential data exposed with the changes.
Summary by CodeRabbit
New Features
traits3
, allowing for more detailed representation of user traits in data transformation.Bug Fixes
Documentation